[Perf] Avoid tx root derivation during block generation#2577
Conversation
vicsn
changed the title
| } | ||
|
|
||
| /// Returns the number of program functions in the deployment. | ||
| pub fn len(&self) -> usize { |
There was a problem hiding this comment.
is len making it obvious that this refers to the number of functions in the program? otherwise num_functions might be a better pick; the same comment applies to is_empty
There was a problem hiding this comment.
Indeed its a bit of an odd naming without context, though it is not unnatural. The Deployment contains just one vector which equals the number of functions/verifying_keys/certificates.
I mainly did this to have consistent naming with Execution::len, where it refers to the number of transitions. This PR then uses both deployment.len() and execution.len() to determine at which "index" to insert an additional Fee transition.
I'd be keen to hear more reviewer's thoughts on this.
There was a problem hiding this comment.
Overall, I'd opt for a more precise naming for clarity. But this is fine as well.
| /// Returns the Merkle tree for the given execution tree and fee. | ||
| pub fn transaction_tree( | ||
| mut execution_tree: TransactionTree<N>, | ||
| fee_index: usize, | ||
| fee: &Fee<N>, | ||
| ) -> Result<TransactionTree<N>> { | ||
| // Construct the transaction leaf. | ||
| let leaf = TransactionLeaf::new_fee(u16::try_from(fee_index)?, **fee.transition_id()).to_bits_le(); | ||
| // Compute the transaction tree. | ||
| execution_tree.append(&[leaf])?; | ||
|
|
||
| Ok(execution_tree) | ||
| } | ||
|
|
There was a problem hiding this comment.
There is now technically 2 paths to get a transaction tree, which can be a bit confusing.
- Creating a tree directly with a deployment/execution + fee
- Creating a deployment/execution tree and then concatenating the fee
Is there a way to unify these (where one relies on the other) into a single function? Also for safety, transaction_tree can be made private.
There was a problem hiding this comment.
Is there a way to unify these (where one relies on the other) into a single function?
Good one, see: 1f2b23e
Also added explicit types for the intermediary trees, to avoid future developers mistaking the outputs: b05b4f1
I'm on the fence whether new DeploymentID and ExecutionID types (similar to TransactionID) would be a net benefit. This would avoid accidental misinterpretation, but in constrat to TransactionID, we don't "need" those new types in most of the codebase.
Also for safety, transaction_tree can be made private.
Doesn't work unfortunately, because its not a "method" taking &self.
|
Great find! We can save the construction of a depth 5 tree for each transaction. My one note was exploring the possibility of a cleaner/more unified design. |
|
|
||
| /// Returns the Merkle tree for the given 1. transaction or deployment tree and 2. fee. | ||
| pub fn transaction_tree( | ||
| mut transaction_tree: TransactionTree<N>, |
There was a problem hiding this comment.
| mut transaction_tree: TransactionTree<N>, | |
| mut deployment_or_execution_tree: TransactionTree<N>, |
3 participants
Motivation
Transaction, deployment and execution root derivation take up 90% of compute of block generation when excluding transaction verification. Fortunately, the ids can be cached at practically 0 cost.
If this is approved, I'll also apply the change to
to_unconfirmed_idand potentially other calls toto_deployment_idandto_execution_id.Test Plan